Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Android: Adjust border radius to maximum of half the smallest side #17514

Closed
wants to merge 1 commit into from

Conversation

yedidyak
Copy link
Contributor

@yedidyak yedidyak commented Jan 10, 2018

Motivation

Currently, if a View has a border radius set to larger than the size of the View, and there s a borderWidth set, there is a very serious performance problem on Android.

Test Plan

This can be verified easily by making any view with a thin border, and watching the Android GPU performance or memory as it is rendered. Native memory allocation will immediately max out, and the GPU render profiler will look like this:

screenshot_1515418843

Memory:
screen shot 2018-01-08 at 16 44 25

With the fix, these problems disappear.

Release Notes

[ANDROID] [BUGFIX][View] - Treat larger than possible border radius as meaning 50% of the smallest side

@facebook-github-bot facebook-github-bot added CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed. cla signed labels Jan 10, 2018
@wodin
Copy link

wodin commented Jan 11, 2018

These test failures are unrelated to this pull request.
The test-android one seems to be because ReactCommon/yoga/BUCK is missing the following:

include_defs("//ReactCommon/DEFS")

@facebook-github-bot
Copy link
Contributor

@yedidyak I tried to find reviewers for this pull request and wanted to ping them to take another look. However, based on the blame information for the files in this pull request I couldn't find any reviewers. This sometimes happens when the files in the pull request are new or don't exist on master anymore. Is this pull request still relevant? If yes could you please rebase? In case you know who has context on this code feel free to mention them in a comment (one person is fine). Thanks for reading and hope you will continue contributing to the project.

@yedidyak
Copy link
Contributor Author

@hramos @grabbou ^

This is being used in production as a patch in Wix.

@grabbou
Copy link
Contributor

grabbou commented Feb 12, 2018

Looks good to me, @janicduplessis and @emilsjolander would you mind looking at this?

@grabbou
Copy link
Contributor

grabbou commented Feb 12, 2018

For the test failures, please rebase your branch as the master is passing :)

@janicduplessis
Copy link
Contributor

Does it also work when setting a specific corner border radius like bottomLeftBorderRadius?

I find the bug kind of weird though, would prefer to find what is actually causing the high memory usage. Have you tried looking at what the path we are drawing looks like to make sure that is ok?

@yedidyak
Copy link
Contributor Author

I rebased, thanks.

@janicduplessis The end result looked correct, but the bounds of the ReactViewBackgroundDrawable expanded to huge numbers in all directions. I tried to analyse what was happening to cause that, I don't remember exactly what I found but the algorithm simply doesn't expect larger than possible border radii.

@janicduplessis
Copy link
Contributor

@RSNara Any idea about what the best fix would be here since you are the last one to have worked on this? :)

@yedidyak
Copy link
Contributor Author

@janicduplessis Whatever is the issue with the underlying algorithm, limiting the borderRadius to the maximum possible size is still worthwhile, no?

@yedidyak
Copy link
Contributor Author

yedidyak commented Mar 1, 2018

@janicduplessis @grabbou bump

@sdwilsh sdwilsh removed the cla signed label Mar 1, 2018
@sercand
Copy link

sercand commented Mar 14, 2018

After upgrading to React-Native 0.54 our app becomes so slow on because of this issue.

@react-native-bot react-native-bot added Android Ran Commands One of our bots successfully processed a command. labels Mar 14, 2018
@facebook-github-bot
Copy link
Contributor

@yedidyak I tried to find reviewers for this pull request and wanted to ping them to take another look. However, based on the blame information for the files in this pull request I couldn't find any reviewers. This sometimes happens when the files in the pull request are new or don't exist on master anymore. Is this pull request still relevant? If yes could you please rebase? In case you know who has context on this code feel free to mention them in a comment (one person is fine). Thanks for reading and hope you will continue contributing to the project.

@react-native-bot react-native-bot added Android Ran Commands One of our bots successfully processed a command. labels Mar 16, 2018
@react-native-bot react-native-bot added Platform: Android Android applications. Ran Commands One of our bots successfully processed a command. labels Mar 18, 2018
@yedidyak
Copy link
Contributor Author

@janicduplessis @grabbou bump?

@grabbou
Copy link
Contributor

grabbou commented Mar 20, 2018

I'll leave the final word to @janicduplessis as he seemed to be more active than me in this thread.

@almostintuitive
Copy link

I assume this would fix:
#17267
#17311
#17144
#17224

And also help us greatly!

Is there any chance it can be approved/merged?
@janicduplessis @grabbou

@yedidyak says they have been using it in production for a while.

@atlanteh
Copy link
Contributor

@janicduplessis can you please have a look?

@mezod
Copy link

mezod commented Apr 26, 2018

+1

@atlanteh
Copy link
Contributor

I'm not sure, but I think this PR can be closed, as the issue #17267 has been fixed which might be the root cause.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Bug CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed. Platform: Android Android applications. Ran Commands One of our bots successfully processed a command.
Projects
None yet
Development

Successfully merging this pull request may close these issues.